-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add replies collection #876
Conversation
Awesome 😍 |
@Menrath Is this ready to merge at least as a first attempt? |
I would love to see some more unit tests, but besides that I feel from my point of view I have nothing to add atm :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems all good to me.
if ( is_local_comment( $comment ) ) { | ||
continue; | ||
} | ||
$comment_meta = \get_comment_meta( $comment->comment_ID ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "if elseif" clause is now present 3-5 times in the code, sometimes slightly different. I have not fully understood why it is slightly different in some cases. But it seems this could be modularised into a function somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfefferle Should someone have a look at this before this PR gets merged? I could find time in the next days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but be aware that there are two different usecases:
- We want to have the ID with URL as fallback.
- We want to have the URL with ID as fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
769e434 added a static function to the Comment util class, that tries to get the link via the comments meta. At some points I thought better error-catching could be possible, but I think this is beyond the scope of this PR. We could also just merge the current state, and revert the commit and add another PR for refactoring the code for comment related stuff.
and maybe there are other fallbacks coming
thanks a lot @Menrath ! |
@pfefferle You're welcome! :) I also really like the last changes, splitting the functionality into the |
Add replies collection to posts and federated comments.
Solves Issue #138
Description
The replies collection offer a neat way to sync replies over multiple instances. Mostly on demand and sometimes also client side on demand. They are not yet frequently used because of the moderation issues that arise with them. But since WordPress already got a good approve-system for comments this should not be too problematic here.
Rest Endpoint
To make sure the ID's of the collection and the collections page are resolveable.
It also adds a new rest endpoint
/activitypub/1.0/(posts|comments)/<id>/replies
which returns aCollection
. Adding the parameter?page=0
returns the CollectionPage directly.I decided not to use something like
<the_permalink>/replies
because it is only needed foractivity+json
and has no frontend view. Seemed consistent to me.Unit tests:
I started writing new unit tests, but was not sure how deep to test this. I'm happy for some discussions.
Manual testing:
Write federated comments for a post, or receive comments and make sure those comments are approved.
When querying the ActivbityPub's JSON representation of the post the replies key exists, e.g.:
Changing the global option
comments_per_page
to for example "3" would change the response to this:Note that the items only contains 2 IDs. This is because the code currently uses core functions to query the paginated comments and later filters out the local-only ones. Because we have more pages also the key
next
is present.It (
https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=1
) resolves to:Unapproved or local only comments should never be listed.
Discussion points